-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve parsing of the ADF15 metadata for H-like ions. #425
Improve parsing of the ADF15 metadata for H-like ions. #425
Conversation
if not config: | ||
raise RuntimeError("Unable to parse ADF15 metadata.") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. More obvious failure modes are good.
cherab/openadas/parse/adf15.py
Outdated
if not config: # try hydrogen header (works for 'bnd' files) | ||
config = _scrape_metadata_hydrogen(file, element, charge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a risk of this inadvertently parsing a different header file incorrectly if it fails on the hydrogen-like parser? I think it would be better to explicitly handle the 'bnd' files here as a special case (with a comment about why it's a special case), and for anything else let it fall through and error until we can manually verify that the same workaround can be applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found any files in ADAS other than those with the "bnd" suffix affected by this issue. Now I did an explicit check on the file name, but since ADAS may update the "bnd" files in the future, the parser tries to read the metadata in "hydrogen-like" format first, and if that fails, then for "bnd" files, it reads the metadata in "hydrogen" format.
This PR addresses the issue raised in #424. Some ADAS ADF15 (PEC) data files for hydrogen-like ions actually have "hydrogen" and not "hydrogen-like" format of the metadata.
While the user can specify the header format in
install_adf15()
andparse_adf15()
, theparse_adf15()
can also try to parse the metadata in "hydrogen" format, if parsing in "hydrogen-like" format fails and theheader_format
is not specified by the user.Also, if parsing of the ADF15 metadata fails,
parse_adf15()
does not raise an error andinstall_adf15()
runs silently without updating the repository. This behaviour is confusing to the user and should be fixed, which is done in this PR.